+1 to get this done as soon as possible

great work @kinglozzer

I think it’s probably important that we document this change if it’s accepted, it may be quite confusing to developers otherwise if images disappear (until we set up CMS messages or something).

Would reference/image.md be the best place for this, or would it better to create a topics/image.md? Documentation on this seems more like a ‘topic’ than ‘reference’ to me, but then we’d have two separate documents for the same subject.

bump

I don’t wanna spend any more time on this until I get a nod of approval from a core committer. Any other comments/criticisms are also welcome of course! 😉

bump

can we please get this merged asap?
Especially with those clients we deploy to shared hosting, and the ever increasing image resolution, this is becoming more and more an issue.
I have clients uploading images with a width of 6k, and they can't even delete them anymore because after the error occurred, you never get to a point in the cms where you could delete the image again.

bumpity bump :)

Could this (or any other proposed fix for this issue) make it into 3.2? Not sure how far off we are.

Thanks Simon, I’d not considered that there might be inconsistencies with __destruct() like that. I’m trying to think of an alternative, but it’s quite tricky without making assumptions about how GDBackend might be used: ideally this would also cater for people who use GDBackend outside of the context of Image.

I guess you could argue that we can’t really be responsible for this - we just need to try to prevent, for example, parts of the CMS being inaccessible.

So, should we:

• A. Not worry about how people may use GDBackend and instead only focus on Image and move some of the logic (i.e. the SS_Cache/stored manipulations logic) into there
• B. Try to find an alternative to __destruct() and try to keep this as GD-specific as possible
• C. Forget the __destruct() stuff, just add the approximate memory calculation to GDBackend and see if that resolves the majority of people’s issues

sounds like a tricky issue.

I for one actually do use GBBackend without image as well on rare occasions, and I like to cleanly separate things if possible, but I see that there might not be a way, or it might be a lot harder, so I am fine with B.

Not sure if C is a good solution, but I am not that much into the the whole issue that I could speak to that (I don't know how accurate the approximation is, or how likely/unlikely it is that an error still occurs).
But if we can get C into core significantly faster than A or B; I would love to see C merged and A or B developed further.

A couple of options:

• Remove items from the cache when writeToFile is called
• Remove items from the cache whenever returning from one of the modifier methods

Given that it's not actually that important to remove items from the cache, I'd prefer the first option.

I’ve moved the cache cleaning to writeToFile() and updated a couple of other minor points.

@simonwelsh I’ve added $this->fail() to the unit tests. I wasn’t sure of the correct usage of it: it seems that a try/catch block catches the failure, so I’ve added an assertion for the exception message to make sure it’s highlighted. Is that correct?

Would be better to throw a more specific exception (so, a subclass) and then catch that so that the PHPUnit exception doesn't get caught.

Tests updated :)

Green button pushed! Now my slides are out of date...

awesome!
great work guys!

Thanks @simonwelsh